Error message improvements for unions with identical discriminants#42598
Error message improvements for unions with identical discriminants#42598DanielRosenwasser wants to merge 3 commits intomainfrom
Conversation
DanielRosenwasser
added
the
Experiment
DanielRosenwasser
requested review from
ahejlsberg,
andrewbranch and
weswigham
DanielRosenwasser
left a comment
DanielRosenwasser
left a comment
There was a problem hiding this comment.
Looking at this again, it's way more arguable. I think part of the approach that might be failing is that we're stopping at the first type, and not continuously refining the union of types given to us.
| foo2({ | ||
| type2: 'y', | ||
| ~~~~~~~~~~ | ||
| !!! error TS2345: Argument of type '{ type2: "y"; value: "done"; method(): void; }' is not assignable to parameter of type 'X2 | Y2'. | ||
| !!! error TS2345: Object literal may only specify known properties, but 'type2' does not exist in type 'X2'. Did you mean to write 'type1'? | ||
| value: 'done', | ||
| method() { | ||
| this; | ||
| this.value; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This is a new error - I'd argue this is actually a regression.
There was a problem hiding this comment.
Yeah, isn’t this perfectly assignable to Y2?
| ~~~~~~~~~~~ | ||
| !!! error TS2322: Type '{ p1: "left"; p2: true; p3: number; p4: string; }' is not assignable to type 'DisjointDiscriminants'. | ||
| !!! error TS2322: Object literal may only specify known properties, and 'p4' does not exist in type '{ p1: "left"; p2: true; p3: number; }'. |
There was a problem hiding this comment.
This one is definitely an improvement.
| amb = { tag: "A", y: 12 } | ||
| ~~~~~ | ||
| !!! error TS2322: Type '{ tag: "A"; y: number; }' is not assignable to type 'Ambiguous'. | ||
| !!! error TS2322: Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'. | ||
| amb = { tag: "A", x: "hi", y: 12 } | ||
| ~~~~~ | ||
| !!! error TS2322: Type '{ tag: "A"; x: string; y: number; }' is not assignable to type 'Ambiguous'. | ||
| !!! error TS2322: Object literal may only specify known properties, and 'y' does not exist in type '{ tag: "A"; x: string; }'. |
There was a problem hiding this comment.
These are...ambiguous! Unclear if they should error.
There was a problem hiding this comment.
Wait, why should they be errors? Particularly the previous error on amb = { tag: "A", y: 12 } seems super wrong. But this one too seems like excess property checking is a little overzealous. For non-discriminated unions of objects, excess property checking only complains about properties non present in any constituent. Here, I would think the same rule would apply after we have filtered out the B and C constituents. So you should be allowed to supply x: string and y: number, but not z: boolean.
| ~~~~~~~~~ | ||
| ~~~~~ |
| amb = { tag: "A", z: true } | ||
| ~~~ | ||
| ~~~~~~~ | ||
| !!! error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type 'Ambiguous'. | ||
| !!! error TS2322: Type '{ tag: "A"; z: true; }' is not assignable to type '{ tag: "B"; z: boolean; }'. | ||
| !!! error TS2322: Types of property 'tag' are incompatible. | ||
| !!! error TS2322: Type '"A"' is not assignable to type '"B"'. | ||
| !!! error TS2322: Object literal may only specify known properties, and 'z' does not exist in type '{ tag: "A"; x: string; }'. |
There was a problem hiding this comment.
This one is sort of an improvement - ideally, we'd be able to say that z was in another branch.
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at a3b8ad2. You can monitor the build here. |
|
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
This experiment is pretty old, so I'm going to close it to reduce the number of open PRs. |
4 participants
This PR would fix #40934, but we will be changing the core logic used in #42556.
However, I'm opening this because I wanted to point out the sorts of
improvementschanges we can see though if we're willing do a little extra work to track multiple object types with identical discriminants though. That logic can possibly be incorporated into #42556.